Skip to content

feat: add stats/incr/nanmrss#6197

Merged
Planeshifter merged 19 commits into
stdlib-js:developfrom
jalajk3004:nanmrss
Jul 7, 2026
Merged

feat: add stats/incr/nanmrss#6197
Planeshifter merged 19 commits into
stdlib-js:developfrom
jalajk3004:nanmrss

Conversation

@jalajk3004

Copy link
Copy Markdown
Contributor

Resolves #5607

Description

What is the purpose of this pull request?

This pull request:

  • {{TODO: add description describing what this pull request does}}

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@stdlib-bot stdlib-bot added Statistics Issue or pull request related to statistical functionality. Good First PR A pull request resolving a Good First Issue. Needs Review A pull request which needs code review. labels Mar 19, 2025
@stdlib-bot

Copy link
Copy Markdown
Contributor

Hello! 👋

We've noticed that you've been opening a number of PRs addressing good first issues. Thank you for your interest and enthusiasm!

Now that you've made a few contributions, we suggest no longer working on good first issues. Instead, we encourage you to prioritize cleaning up any PRs which have yet to be merged and then proceed to work on more involved tasks.

Not only does this ensure that other new contributors can work on things and get ramped up on all things stdlib, it also ensures that you can spend your time on more challenging problems. 🚀

For ideas for future PRs, feel free to search the codebase for TODOs and FIXMEs and be sure to check out other open issues on the issue tracker. Cheers!

@stdlib-bot

stdlib-bot commented Mar 19, 2025

Copy link
Copy Markdown
Contributor

Coverage Report

Package Statements Branches Functions Lines
stats/incr/nanmrss $\\color{green}155/155$
$\\color{green}+0.00\\%$
$\\color{green}8/8$
$\\color{green}+20.00\\%$
$\\color{green}2/2$
$\\color{green}+0.00\\%$
$\\color{green}155/155$
$\\color{green}+0.00\\%$

The above coverage report was generated for the changes in this PR.

@jalajk3004 jalajk3004 changed the title feat added stats/incr/nanmrss feat: add stats/incr/nanmrss Mar 19, 2025

@hrshya hrshya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The img directory is also missing in docs.


#### incrnanmrss( window )

Returns an accumulator `function` which incrementally computes a moving [residual sum of squares][residual-sum-of-squares]. The `window` parameter defines the number of values over which to compute the moving [residual sum of squares][residual-sum-of-squares].

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to add , ignoring NaN values. at the end.

/**
* If provided arguments, returns an updated residual sum of squares; otherwise, returns the current residual sum of squares, ignoring `NaN` values.
*
* ## Notes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from L26-29

* @returns {(number|null)} residual sum of squares or null
*/
function accumulator( x, y ) {
if ( arguments.length === 0 ) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using two if statements. This can be done in a single if statement. (L79-84)

@jalajk3004

Copy link
Copy Markdown
Contributor Author

@hrshya you can review this PR again, i have resolved the errors

@hrshya

hrshya commented Mar 20, 2025

Copy link
Copy Markdown
Contributor

@hrshya you can review this PR again, i have resolved the errors

On it!

@hrshya hrshya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jalajk3004 Can you also change the description to:

adds stats/incr/nanmrss .

@Planeshifter Planeshifter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better late than never... thanks for opening a PR!


# incrnanmrss

> Compute a moving [residual sum of squares][residual-sum-of-squares] (RSS) incrementally, ignoring `NaN` calues.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo.

Suggested change
> Compute a moving [residual sum of squares][residual-sum-of-squares] (RSS) incrementally, ignoring `NaN` calues.
> Compute a moving [residual sum of squares][residual-sum-of-squares] (RSS) incrementally, ignoring `NaN` values.

if ( arguments.length === 0 || isnan( x ) || isnan( y ) ) {
return mrss();
}
return mrss( ( ( isnan( x ) ) ? 0 : x), ( ( isnan( y ) ) ? 0 : y ) );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code. Line 79 already returns early when either x or y is NaN, so the isnan ternaries are unreachable.

Suggested change
return mrss( ( ( isnan( x ) ) ? 0 : x), ( ( isnan( y ) ) ? 0 : y ) );
return mrss( x, y );

* @returns accumulator function
*
* @example
* var accumulator = incrmrss( 3 );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong function name.

Suggested change
* var accumulator = incrmrss( 3 );
* var accumulator = incrnanmrss( 3 );

v1 = ( bernoulli( 0.8 ) < 1 ) ? NaN : uniform( 0.0, 100.0 );
v2 = ( bernoulli( 0.8 ) < 1 ) ? NaN : uniform( 0.0, 100.0 );
rss = accumulator( v1, v2 );
console.log( '%s\t%s\t%s', ((Number.isNaN(v1)) ? 'NaN' : v1.toFixed(3)), ((Number.isNaN(v2)) ? 'NaN' : v2.toFixed(3)), (((rss !== null) && (!Number.isNaN(rss))) ? rss.toFixed(3) : 'null'));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Number.isNaN is ES6. Examples must be ES5. Also missing spaces inside parentheses. See mrss/examples/index.js.

@Planeshifter Planeshifter added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Mar 9, 2026
@Planeshifter Planeshifter requested a review from a team July 7, 2026 04:53
@Planeshifter Planeshifter dismissed their stale review July 7, 2026 04:53

Requested changes applied in the maintainer cleanup commit: removed the dead-code NaN ternaries, fixed the incrmrss -> incrnanmrss example, replaced ES6 Number.isNaN in the example, and fixed the README typo/annotations. Dismissing the stale review.

@Planeshifter Planeshifter force-pushed the nanmrss branch 2 times, most recently from 3880010 to 1fef29f Compare July 7, 2026 05:01
@stdlib-bot stdlib-bot added the Needs Review A pull request which needs code review. label Jul 7, 2026
- Remove unreachable NaN-guard ternaries in the accumulator return (the early guard already returns on NaN); forward x and y directly
- Fix incrmrss -> incrnanmrss in the index.d.ts example
- Use @stdlib/math/base/assert/is-nan instead of ES6 Number.isNaN in the example and add conventional spacing
- Fix the README 'calues' typo and correct misleading window annotations for ignored NaN pairs
- Drop backticks from package.json description; bump copyright year to 2026

@Planeshifter Planeshifter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@Planeshifter Planeshifter added the Ready To Merge A pull request which is ready to be merged. label Jul 7, 2026
@stdlib-bot stdlib-bot removed Needs Review A pull request which needs code review. Needs Changes Pull request which needs changes before being merged. labels Jul 7, 2026
@stdlib-bot

Copy link
Copy Markdown
Contributor

PR Commit Message

feat: add `stats/incr/nanmrss`

PR-URL: https://github.com/stdlib-js/stdlib/pull/6197
Closes: https://github.com/stdlib-js/stdlib/issues/5607

Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Reviewed-by: Philipp Burckhardt <pburckhardt@outlook.com>

Please review the above commit message and make any necessary adjustments.

@Planeshifter Planeshifter merged commit 29c447e into stdlib-js:develop Jul 7, 2026
32 checks passed
@stdlib-bot stdlib-bot removed the Ready To Merge A pull request which is ready to be merged. label Jul 7, 2026
@kgryte

kgryte commented Jul 7, 2026

Copy link
Copy Markdown
Member

@Planeshifter Not sure why, but all the linting in this PR was a no-op in CI: https://github.com/stdlib-js/stdlib/actions/runs/28843282273/job/85541581494?pr=6197

Worthwhile to always merge in the latest develop before fixing up these older PRs.

@kgryte

kgryte commented Jul 7, 2026

Copy link
Copy Markdown
Member

I noticed this, as CI failed after merging to develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Good First PR A pull request resolving a Good First Issue. Statistics Issue or pull request related to statistical functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RFC]: add stats/incr/nanmrss

5 participants